Can each of these be on a new line - for when we add new conditions in the future

I will push a change to normalize to that style across *_types.go

I prefer PropagateConfigurationStatus

I will push a change with this rename.

I prefer PropagateRouteStatus

I will push a change with this rename.

I would suggest more granularity and test the scenarios below. Let me know if it's too pedantic. I'm suggesting this because there's no test for when a configuration fails and recovers. And there's some overlap testing the route failure flow in TestTypicalServiceFlow and TestRouteFailurePropagation

Each bullet point would be a scenario to test given the conditions when ...

service happy flow

when the service ready condition is unknown

• route ready condition becomes true
• configuration ready condition becomes true
• both route and configuration ready condition becomes true

failure propagation

when the service ready condition is true
and both route & configuration ready conditions are true

• route ready condition becomes false
• configuration ready condition becomes false

configuration recovery

when the service ready condition is false
and route ready condition is true
and configuration ready condition is false

• configuration ready condition becomes true

route recovery

when the service ready condition is false
and route ready condition is false
and configuration ready condition is true

• route ready condition becomes true

I opened #1224 to track this. This is a great list, thanks.

Can we name this serviceInformer for clarity

Pushing change

Multiple lines

Pushing a change.

serviceLister

Pushing change.

Use https://github.com/kubernetes/apimachinery/blob/master/pkg/api/equality/semantic.go to make sure they are semantically the same

One reason being cmp.Diff doesn't seem to equate empty and nil maps as equal. Hence the need for these cmp options https://godoc.org/github.com/google/go-cmp/cmp/cmpopts

Agreed, though IMO we should continue using cmp.Diff in tests.

I left the cmp.Diff for logging to the controller logs, but switched the actual comparison the equality.Semantic.DeepEqual. Will add this to my change incorporating feedback.

@grantr I agree tests should use cmp.Diff

avoid cmp.Diff as mentioned above

I'm not opposed to this solution, but seems like defaulter-gen might be cleaner. If only defaulter-gen were documented...

This appears to skip reconciling the configuration when it's just been created. I'm curious if there's a reason not to reconcile the configuration immediately. The informer cache won't have it, but that's likely not a problem because we already have the latest state as returned by Create.

It's true that the service will be re-added to the workqueue almost immediately (depending on latency of the configuration informer), but if the workqueue is long there might be a noticeable delay before it comes back.

I believe that all this would end up reconciling is changes made by mutating webhooks (or the API Server). It's a big problem if webhooks make changes that disagree with our reconciliation (we'll flip-flop every resync), so I'm not sure I see the value in this. I may be naive here, but what would we miss by not doing this?

My concern is that I don't see any documentation or test that createConfiguration, reconcileConfiguration, and the webhook must all result in exactly the same object, but the code relies on that being true in subtle ways. It would be easy to change only one of the create or reconcile methods without changing the other, since things would still appear to work in practice (because of the immediate second reconcile).

Probably the easiest solution for this (minus the webhook issue) is to add a unit test that diffs the output of createConfiguration and reconcileConfiguration.

An alternative is to allow reconcileConfiguration to take a nil Configuration:

func (c *Controller) reconcileConfiguration(service *v1alpha1.Service, config *v1alpha1.Configuration) (*v1alpha1.Configuration, error) {
  desiredConfig := MakeServiceConfiguration(service)
  if config == nil {
    return c.ElaClientSet.ServingV1alpha1().Configurations(service.Namespace).Create(config)
  else {
    desiredConfig.Spec.Generation = config.Spec.Generation
    if equality.Semantic.DeepEqual(desiredConfig.Spec, config.Spec) != {
      config.Spec = desiredConfig.Spec
      return c.ElaClientSet.ServingV1alpha1().Configurations(service.Namespace).Update(config)
    }
  }
  return config, nil
}

The webhook issue is trickier to solve fully, but could at least be mitigated using defaulter-gen (because the generated defaulters would be defined in one place and used by both webhook and controller).

Same question as above re: not reconciling the route immediately.

Agreed, though IMO we should continue using cmp.Diff in tests.

I think I'd collapse these into one line without setting configClient, but I'm fine with this approach too.

Looks like I already did this in configuration.go, will add this to the change I'm pushing.

This function (and createRoute below it) do so little I wonder if it might be clearer to inline them into the reconcile.

I think I have a mild preference for this slightly more readable method name for readability of the core reconciliation method, even if it is now one line :)